Introducing content-filtering topics#901
Conversation
makes makes this feature available to rclnodejs developers. node.js - added contentFilter to Options - added static getDefaultOptions() - updated createSubscription() to support contentFilter node.d.ts - added content-filter types subscription.js - isContentFilteringEnabled() - setContentFilter() - clearContentFilter() subscription.d.ts - updated with content-filter api rcl_bindings.cpp - added content-filtering to CreateSubscription() rmw.js - new class for identifying the current ROS middleware test-subscription-content-filter.js - test cases for content-filters test/blocklist.json - added test-subscription-content-filter.js for Windows and Mac OS examples: - publisher-content-filtering-example.js - subscription-content-filtering-example.js package.json - added build/rebuild scripts for convenience
wayneparrott
force-pushed
the
subscription_content_filter
branch
from
78cc8b5 to
6c63a81
Compare
minggangw
left a comment
minggangw
left a comment
There was a problem hiding this comment.
Thanks for submitting this huge PR and implementing the content filter feature!
lib/subscription.js
Outdated
| */ | ||
| setContentFilter(contentFilter) { | ||
| if (!contentFilter) return false; | ||
|
|
There was a problem hiding this comment.
Do we need to check this.isContentFilteringEnabled() here?
There was a problem hiding this comment.
re: isContentFilteringEnabled(), this will only return true if both:
- the RMW implementation support content-filtering topics (CTF) and
- *the subscription is currently configured with a valid content-filter.
So the use of isContentFilteringEnabled() is limited to identifying if a subscription currently configured with a valid content-filter. I'm not aware of an rcl level api for identifying if an RMW implementation support CTFs.
I found this bit of low level code in the fastrtps impl that implements isContentFilteringEnabled().
@minggangw do you think it would be more clear to developers if we renamed isContentFilteringEnabled() to something like: hasContentFilter()?
I originally implemented without the guard condition like this:
setContentFilter(contentFilter) {
return contentFilter && contentFilter.expression
? rclnodejs.setContentFilter(this.handle, contentFilter)
: this.clearContentFilter();
}At the rcl level, passing null or empty string for conentFilter.expression will clear the current content-filter. I propose that we use the implementation above. Thoughts?
There was a problem hiding this comment.
It sounds that hasContentFilter() is more clear.
At the rcl level, passing null or empty string for conentFilter.expression will clear the current content-filter. I propose that we use the implementation above. Thoughts?
Agree, we can remove if (!contentFilter) return false; here, thanks
minggangw
left a comment
minggangw
left a comment
There was a problem hiding this comment.
LGTM! Thanks for making this big change!
* ROS Humble introduced the content-filtering topics feature. This PR makes makes this feature available to rclnodejs developers. node.js - added contentFilter to Options - added static getDefaultOptions() - updated createSubscription() to support contentFilter node.d.ts - added content-filter types subscription.js - isContentFilteringEnabled() - setContentFilter() - clearContentFilter() subscription.d.ts - updated with content-filter api rcl_bindings.cpp - added content-filtering to CreateSubscription() rmw.js - new class for identifying the current ROS middleware test-subscription-content-filter.js - test cases for content-filters test/blocklist.json - added test-subscription-content-filter.js for Windows and Mac OS examples: - publisher-content-filtering-example.js - subscription-content-filtering-example.js package.json - added build/rebuild scripts for convenience * Delete obsolete ./test.js * implements recommended PR feedback
2 participants
ROS Humble introduced the content-filtering topics feature. This PR makes this feature available to rclnodejs developers.
Resources
DDS 1.4 specification, Annex B
Content-filtering tutorial
ros2/design#282
subscription.h
Supported RMWs
Content-filtering is supported by the following RMWs as of the Humble release:
Note-1: rmw_cyclonedds_cpp does NOT support content-filtering yet.
Note-2: Our experience is that content-filtering on rmw_fastrtps is only works on the Linux versions of ROS Humble and Rolling. Therefore, the test/blocklist.json excludes the test-subscription-content-filter.js test on Windows and Mac.
Key changes
node.js
node.d.ts
subscription.js
subscription.d.ts
rcl_bindings.cpp
rmw.js
test-subscription-content-filter.js
examples:
package.json